Skip to content

feat(sdk-coin-sol): implement staking activate for jito #6502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

haritkapadia
Copy link

@haritkapadia haritkapadia commented Jul 21, 2025

Description

This PR updates StakingActivate to support Solana staking with JitoSOL tokens. To do so, it adds support for the depositSol solana/spl-stake-pool instruction.

Issue Number

TICKET: SC-2314

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

A new unit test to build the transaction has been added to modules/sdk-coin-sol/test/unit/transactionBuilder/stakingActivateBuilder.ts.

An example script has been added to examples/ts/sol/jito-stake.ts. Here is a transaction produced by running the example script.

@haritkapadia haritkapadia marked this pull request as ready for review July 21, 2025 23:28
@haritkapadia haritkapadia requested a review from a team as a code owner July 21, 2025 23:28
@haritkapadia haritkapadia marked this pull request as draft July 25, 2025 21:25
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we don't even have an _isTestnet flag and the parameters that depend on the flag (JITO reserve stake account address and manager fee address) can be set using coin information or token information.

* @param {TransactionInstruction} instruction
* @returns {DepositSolParams}
*/
export function decodeDepositSol(instruction: TransactionInstruction): DepositSolParams {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haritkapadia haritkapadia marked this pull request as ready for review July 25, 2025 22:44
@haritkapadia haritkapadia requested review from a team as code owners July 25, 2025 22:44
Copy link
Contributor

@evanzbitgo evanzbitgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comments in a mirrored PR #6501

@haritkapadia
Copy link
Author

shouldn't we add the sol dependency under sol module instead?

Fixed.

use enum stakingMethod for default, marinade and jito instead to keep it clean

We've used isJito in staking-service and wallet-platform, so I'm following the convention.

Why import avax lib

Fixed. I was using it for encoding base58. Now I'm using the bs58 package.

Is this is helper lib? If yes, then jitoStakePoolOperations.ts might be a less confusing name

Fixed. Originally I called it future because it was directly copying functions from future versions of spl-stake-pool. Now they are not copies, even though they are functions that may not be needed if the spl-stake-pool library ever updates. See #6502 (review).

In other coins, we typically do not have this flag. It's caller's responsibility to decide mainnet or testnet configs to use.

Do you have any ideas where to store the following information? Should it be a new field in StakingActivate? Or somewhere else...

https://github.com/BitGo/BitGoJS/pull/6502/files#diff-b7626bc12f2143ec01101785befc38fb90ef4450273d22305577c9e79a22c343R267-R273

@haritkapadia haritkapadia requested a review from evanzbitgo July 29, 2025 15:22
@@ -13,9 +13,12 @@ export class StakingActivateBuilder extends TransactionBuilder {
protected _stakingAddress: string;
protected _validator: string;
protected _isMarinade = false;
protected _isJito = false;
protected _isTestnet: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this _isTestnet and isTestnet in StakingActivate. It does not look clean.

In TransactionBuilder, you can save _coinConfig locally and passdown into solInstructionFactory and then decide what those configs are.

@@ -114,6 +115,8 @@ export interface StakingActivate {
amount: string;
validator: string;
isMarinade?: boolean;
isJito?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create an enum for marinade, jito. There might be new staking types later. You can refactor in a separate PR with WP changes

@evanzbitgo
Copy link
Contributor

Pasting AI review results

🚨 CODE REVIEW - CRITICAL ISSUES FOUND

OVERALL ASSESSMENT: NEEDS MAJOR REVISION ❌

This PR has significant architectural problems and violates several best practices. DO NOT MERGE without addressing these issues.


🔴 CRITICAL ISSUES

  1. Poor Parameter Design - isTestnet is Problematic

Location: src/lib/iface.ts:91
isTestnet?: boolean; // ❌ BAD: Manual network specification

Problems:

  • ❌ Manual network detection is error-prone - developers must remember to set this correctly
  • ❌ Violates DRY principle - network type is already available in CoinConfig
  • ❌ Type safety risk - no guarantee isTestnet matches actual network configuration
  • ❌ Inconsistent with codebase patterns - other builders derive network automatically

Fix Required: Remove this parameter and derive network automatically from CoinConfig

  1. Inconsistent Implementation Pattern

Location: src/lib/stakingActivateBuilder.ts:21, 41
this._isTestnet = this._coinConfig.network.type === NetworkType.TESTNET; // ✅ Good default
// But then...
this.isTestnet(activateInstruction.params.isTestnet ?? this._coinConfig.network.type === NetworkType.TESTNET); // ❌ Overridable

Problems:

  • ❌ Contradictory logic - sets network automatically but allows manual override
  • ❌ Complex fallback logic that's hard to understand and maintain
  • ❌ Bug potential - override can mismatch actual network
  1. Hardcoded Constants Violate Architecture Principles

Location: src/lib/solInstructionFactory.ts:257-262
const stakePoolReserveStake = new PublicKey(
isTestnet ? JITO_STAKE_POOL_RESERVE_ACCOUNT_TESTNET : JITO_STAKE_POOL_RESERVE_ACCOUNT
);

Problems:

  • ❌ Scattered network-specific logic - should be centralized
  • ❌ Hard to maintain - adding new networks requires changes in multiple places
  • ❌ No abstraction - direct conditional checks throughout codebase

🟡 MODERATE ISSUES

  1. Magic Detection Logic

Location: src/lib/instructionParamsFactory.ts:345-347
...(stakingInstructionsIsJito(stakingInstructions) &&
stakingInstructions.depositSol?.reserveStake.toString() === JITO_STAKE_POOL_RESERVE_ACCOUNT_TESTNET
? { isTestnet: true }
: {}),

Problems:

  • ⚠️ Fragile detection - relies on string comparison of addresses
  • ⚠️ Incomplete logic - only detects testnet, assumes mainnet otherwise
  • ⚠️ Hard to extend - adding new environments breaks this pattern
  1. Missing Error Handling

Location: src/lib/solInstructionFactory.ts:255
if (isJito) {
// No validation that isTestnet is provided for Jito operations

Problems:

  • ⚠️ Missing validation - should assert that network information is available
  • ⚠️ Silent failures possible - could use wrong addresses without warning

🟢 POSITIVE ASPECTS

✅ Good: Proper TypeScript typing throughout
✅ Good: Comprehensive test coverage added✅ Good: Follows existing code style and patterns
✅ Good: Proper error assertions for required parameters


📝 RECOMMENDED REFACTORING

Instead of the current approach, implement:

  1. NetworkConstants Service Pattern:
    class NetworkConstants {
    constructor(private coinConfig: CoinConfig) {}
get jitoStakePoolReserveAccount(): string {
  return this.coinConfig.network.type === NetworkType.TESTNET
    ? JITO_STAKE_POOL_RESERVE_ACCOUNT_TESTNET
    : JITO_STAKE_POOL_RESERVE_ACCOUNT;
}

}

  1. Remove isTestnet parameter entirely
  2. Inject NetworkConstants into builders and factories
  3. Centralize all network-specific logic

🛑 BLOCKING ISSUES FOR MERGE

  • Remove isTestnet parameter from interface
  • Implement automatic network detection
  • Centralize network-specific constants
  • Update all usage sites
  • Verify tests pass with new architecture

Estimated rework: 4-6 hoursRisk level: High (affects core transaction building)

This PR shows promise but needs significant architectural improvements before it can be merged safely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants